Skip to content

signal: describe what the OS handler actually writes, and cover duck-typed re group selectors - #1225

Merged
youknowone merged 2 commits into
mainfrom
gc-decouple
Aug 14, 2026
Merged

signal: describe what the OS handler actually writes, and cover duck-typed re group selectors#1225
youknowone merged 2 commits into
mainfrom
gc-decouple

Conversation

@youknowone

Copy link
Copy Markdown
Owner

Two independent follow-ups from the #1209 review.

signal: the ticker comments named the wrong writer

Three comments — register_ticker, signal_pushback, and the ticker_addr handoff in install_signal_handling — documented the OS signal handler as forcing the ActionFlag ticker negative. It does not. The handler sets the pending bit and arms the eval-breaker's async bit, both lock-free atomics; sync_async_ticker drives the ticker negative later, under the GIL. That split is exactly what keeps the handler async-signal-safe, so the comments now say why the handler must not write that cell.

Comments only; no behaviour change. A reviewer named one of the three sites — the other two carried the same claim.

re: __index__ arm of the group-selector conversion was uncovered

group, __getitem__, start, end, and span take a selector as a number when its type supplies __index__ and as a name otherwise. stdlib_re.py only ever passed literal integers and literal names, so the conversion's __index__ arm — which runs arbitrary Python — was never exercised.

Not included

The review's Critical finding on sre_match_group (an unrooted *const W_SRE_Match held across do_span, which can run arbitrary Python) is refuted. W_SRE_Match is built only by w_sre_match_newW_SRE_Match::allocate_stablealloc_in_oldgen; MiniMark's mark-sweep never moves old-gen objects, and drag_out_root relocates only is_nursery_object_start refs. allocate_stable exists precisely so callers can hold the raw pointer on the Rust stack without rooting it. I did write that "fix" before checking the allocator, then reverted it.

`register_ticker`, `signal_pushback`, and the `ticker_addr` handoff in
`install_signal_handling` all documented the OS handler as forcing the ticker
negative.  The handler sets the pending bit and arms the eval-breaker's async
bit, both lock-free atomics; `sync_async_ticker` is what drives the ticker
negative, under the GIL.

Assisted-by: Claude
`group`, `__getitem__`, `start`, `end`, and `span` take a group selector as a
number when its type supplies `__index__` and as a name otherwise.  The snippet
only passed literal integers and literal names, so the `__index__` arm of the
selector conversion was uncovered.

Assisted-by: Claude
@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@youknowone, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 34 seconds

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 47206ecf-85fc-494a-9d7b-1b82fbf3050c

📥 Commits

Reviewing files that changed from the base of the PR and between 45d3eba and efcb5ef.

📒 Files selected for processing (3)
  • pyre/extra_tests/snippets/stdlib_re.py
  • pyre/pyre-interpreter/src/module/signal/interp_signal.rs
  • pyre/pyre-interpreter/src/module/signal/signalstate.rs

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@youknowone
youknowone merged commit 6fd2f1c into main Aug 14, 2026
8 checks passed
@youknowone
youknowone deleted the gc-decouple branch August 14, 2026 15:06
@github-actions

Copy link
Copy Markdown

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit efcb5ef).
Updated: 2026-08-14T15:09:11.868Z

Files in the reviewed diff
pyre/extra_tests/snippets/stdlib_re.py
pyre/pyre-interpreter/src/module/signal/interp_signal.rs
pyre/pyre-interpreter/src/module/signal/signalstate.rs

1. Regressions to PyPy parity introduced by this patch

None.

2. Other mismatches introduced by this patch

None.

3. Pre-existing mismatches (already present before this patch)

None.

4. Structural adaptations

  • pyre/pyre-interpreter/src/module/signal/signalstate.rs:83 ↔ rpython/translator/c/src/signals.c:112: pyre arms an atomic eval-breaker and transfers it to the ticker at a safe checkpoint, whereas PyPy’s C handler writes the signal ticker directly. This is a Rust async-signal-safety / implementation-language adaptation, not a parity regression. The executable behavior predates this patch; the patch only corrects its documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant